Skip to content

Report parser field mismatches instead of trapping - #7

Merged
RISCfuture merged 3 commits into
masterfrom
fix/uncatchable-parser-traps
Sep 8, 2026
Merged

Report parser field mismatches instead of trapping#7
RISCfuture merged 3 commits into
masterfrom
fix/uncatchable-parser-traps

Conversation

@RISCfuture

Copy link
Copy Markdown
Owner

Two out-of-range array subscripts in the fixed-width parsing path. Both are traps, not throws, so neither can be caught by the errorHandler that NASR.parse(_:withProgress:errorHandler:) is built around: the process dies with SIGILL and the caller gets no diagnostic and no chance to skip the record.

The PCN trap is a live risk, not a hypothetical

parsePavementClassification(_:) split the Pavement Classification field on / and indexed components[1] through components[4] unchecked; only components[0] was guarded.

The call site does wrap the parse:

do {
  pavementClassification = try parsePavementClassification(classStr)
} catch {
  throw FixedWidthParserError.invalidValue(classStr, at: 8)
}

That catch cannot run for an out-of-range subscript.

What makes this current rather than theoretical: the field widened from 11 to 16 characters in the 2026-09-03 airport layout because it is transitioning from ICAO PCN to ICAO PCR. A PCR string is conventionally four-part, where PCN is the five-part number/type/subgrade/tirePressure/determination. The first runway record in any cycle that carries a PCR-shaped value in that field would take the process down. Shorter values trap the same way — a bare 61, or 61//B/X/T, since split drops empty subsequences by default.

The fix guards the component count and throws the Error.invalidPavementClassification(_:) the function already throws elsewhere, so the existing catch routes it to FixedWidthParserError.invalidValue(_:at:) and the record is dropped through the normal error channel.

On omittingEmptySubsequences: false: considered and rejected. It would turn 61//B/X/T into five components with an empty type, which then fails deeper in Classification.require("") — a worse diagnostic than reporting the whole offending value. Keeping the default split means every malformed shape reports the same way, naming the value the FAA actually published.

The CSV path (CSVAirportParser) reads the five pieces as separate columns and was already safe; it is untouched.

The field-count trap is the same failure one layer up

ByteTransformer.applyTo(_:) mapped over slices produced from the FAA's runtime-parsed layout file while indexing fields — the hardcoded, positional transformer list (135 entries for the runway record). Nothing checked that the two agreed:

  • a layout that gains a field traps on fields[index], a fresh SIGILL of exactly the kind fixed in 4.1.1;
  • a layout that loses one is worse: every index past the removal silently reads the neighbouring field's bytes, and the resulting records are wrong with no error anywhere.

Now guarded, throwing a new FixedWidthParserError.fieldCountMismatch(expected:actual:) that names both counts.

Source compatibility: FixedWidthParserError is internal, so adding a case is not source-breaking for consumers. Errors reach consumers wrapped in the public RecordParseError, whose cases are unchanged.

Airport.id documentation

The site number was documented as the field to use "as the LID for an airport can sometimes change." The FAA falsified that in the 2026-09-03 cycle: FAA LID 18AL (LOUISVILLE STAGEFIELD AHP) moved from site number 03329.19 to 00329.19, a corrected digit transposition — permanent, and not re-published under the old key. The site number is now documented as unique within a cycle but not stable across cycles, with the note that persisting either identifier needs a reconciliation step to tell a re-keyed record from a retired one.

Testability change

parsePavementClassification(_:) is now static instead of private. It touches no actor state, so nonisolated-static is the more accurate declaration, and it lets the tests exercise the guard directly rather than standing up an Airport through an 80-parameter initializer to reach the runway record path. It moved above the instance methods to satisfy SwiftLint's type_contents_order; its body is otherwise unchanged apart from the new guard.

Not included

ParserError.truncatedRecord's expectedMinLength reporting the offending field's upper bound rather than the record's declared logical length was raised as an optional cleanup. Left alone: given the parameter is named min length, the field's upper bound is a defensible reading — it is the smallest record that would have contained that field — and changing it would alter the meaning of an existing public error payload for a debatable gain.

Tests

Four cases added to FixedWidthParserTests, all in the existing file:

  • PCN values with one, four, and six components, plus 61//B/X/T, each reporting invalidPavementClassification with the offending value.
  • A well-formed 61/R/B/X/T still parsing to every expected component, so the count guard cannot silently reject valid data.
  • A transformer given one fewer and one more slice than it has fields, reporting fieldCountMismatch with both counts. The "one fewer" case is the one that was never a crash — it covers the silent misread.

Verification

swift build                  Build complete!
swift test                   Test run with 118 tests in 26 suites passed
swiftlint --strict           Found 0 violations, 0 serious in 187 files
swift format lint --strict   clean (repo's CI config)
generate-documentation       Finished building documentation (--warnings-as-errors)

CHANGELOG.md gains an ## [Unreleased] section. No version bump, no tag.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QZ3UWydnavm3g9Gs62Xirb

Two uncatchable traps in the fixed-width path:

- parsePavementClassification indexed the five components of a PCN value
  without checking how many the split produced. The Pavement Classification
  field is widening from 11 to 16 characters for the ICAO PCR transition, and
  a PCR value is conventionally four-part, so a value from an upcoming cycle
  would trap on the first runway record carrying one. The call site wraps the
  parse in do/catch, but an out-of-range subscript is a trap, not a throw, so
  that handler never ran.
- ByteTransformer indexed its compiled-in transformation list by the position
  of a slice produced from the runtime-parsed layout. A layout that gains a
  field trapped; one that loses a field silently read every subsequent value
  from its neighbour.

Both now throw, so the parse error handler sees them and the affected record
is dropped rather than the process.

Airport.id is documented as unique within a single cycle rather than stable
across cycles: the FAA re-keyed FAA LID 18AL from site number 03329.19 to
00329.19 in the 2026-09-03 cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QZ3UWydnavm3g9Gs62Xirb
NASR cycle. It is not a stable identifier across cycles: the FAA
occasionally corrects a site number, just as an airport's ``LID`` can
change. Persisting either value across cycles requires a reconciliation step
to detect a record that has been re-keyed rather than retired.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave these extra notes out of here -- if anything, they should go into a separate document or other location more focused on cycle updates. For this here, just cover the static info.

)
case let .fieldCountMismatch(expected, actual):
return String(
localized: "Layout describes \(actual) fields, but the parser transforms \(expected)"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use format: .number (where supported on this platform)

RISCfuture and others added 2 commits September 8, 2026 14:23
The site number's documentation covers what the identifier is and the cycle
it is unique within; guidance on handling cycle updates belongs elsewhere.

The field count mismatch message renders both counts through `.number` so
they localize.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QZ3UWydnavm3g9Gs62Xirb
Foundation on Linux resolves `String(localized:)` through a plain-`String`
initializer, whose interpolation takes no format style.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QZ3UWydnavm3g9Gs62Xirb
@RISCfuture
RISCfuture merged commit 4e28995 into master Sep 8, 2026
7 checks passed
@RISCfuture
RISCfuture deleted the fix/uncatchable-parser-traps branch September 8, 2026 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant